-
Notifications
You must be signed in to change notification settings - Fork 526
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add switch to allow enabling overrides-exporter ring #3995
Conversation
@@ -24,11 +25,20 @@ | |||
local containerPort = $.core.v1.containerPort, | |||
overrides_exporter_port:: containerPort.newNamed(name='http-metrics', containerPort=$._config.server_http_port), | |||
|
|||
local ringConfig = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our jsonnet support two ring backends: consul (default) and memberlist. Look for querySchedulerRingClientConfig
and try to come up with something similar. Since in this case both lifecycler and ring client is only used by overrides exporter, you don't need two separate configs for the lifecycler and ring client, but a single overridesExporterRingConfig
is enough. Make overridesExporterRingConfig
not local, so that it's overrideable (like querySchedulerRingClientConfig
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I would enable it by default with deployment mode is read-write, so we also see the difference in the jsonnet tests (run make jsonnet-tests
to update the tests output).
f4f007c
to
8219634
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good! I just left a comment to fix the memberlist override, we shouldn't happen if the overrides-exporter ring is disabled.
- -memberlist.bind-port=7946 | ||
- -memberlist.join=dns+gossip-ring.default.svc.cluster.local:7946 | ||
- -overrides-exporter.ring.store=memberlist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct. Shouldn't be set if the overrides-exporter ring is disabled.
d736ff3
to
5b6ff22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
What this PR does
This adds jsonnet config for the ring for overrides-exporters and enables it for the read-write deployment mode.
Do changes like this need a changelog entry?
Which issue(s) this PR fixes or relates to
#3806
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]